-
Notifications
You must be signed in to change notification settings - Fork 104
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[JENKINS-41004] Do not report credentials with IDs masked by nearer folders #83
Conversation
…olders - Also fix a permission but where folder credentials were only available to SYSTEM and not available to authentications with USE_ITEM permission
- Added tests that show it was unnecessary, so reverting back to original behaviour as that matches more closely the System store in credentials plugin
if (ACL.SYSTEM.equals(authentication)) { | ||
while (itemGroup != null) { | ||
if (itemGroup instanceof AbstractFolder) { | ||
if (!((AbstractFolder) itemGroup).getACL().hasPermission(authentication, USE_ITEM)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, you just verified three lines up that authentication
is equal to ACL.SYSTEM
so this seems redundant to me.
🐛 missing test that verifies a folder credential is chosen when there is a system credential with the same id. |
CredentialsMatchers.always())) { | ||
if (!(c instanceof IdCredentials) || ids.add(((IdCredentials) c).getId())) { | ||
// if IdCredentials, only add if we havent added already | ||
// if not IdCredentials, always add |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any non-IdCredentials
, really? Should we just deprecate that possibility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've see one implementation - cannot recall where - so I am not going to pull the plug on them until I have the implementation guidelines written and published
} | ||
} | ||
if (itemGroup instanceof Item) { | ||
itemGroup = Item.class.cast(itemGroup).getParent(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
itemGroup = ((Item) itemGroup).getParent();
- Also need to bump credentials plugin to 2.1.11 to ensure that the stores are identified in the correct sequence
@rsandell we should be good now! (assuming the CI server agrees) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐝
See JENKINS-41044
Relates to jenkinsci/credentials-plugin#77
@reviewbybees